Improve Windows test reliability#9161
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Improves Windows functional-test reliability by isolating extension state, retrying transient filesystem locks, and routing terminal output through injected writers.
Changes:
- Adds cross-platform resilient directory cleanup.
- Isolates extension tests and guards fixture assertions.
- Propagates writers through tool and UX components and documents reliability practices.
Show a summary per file
| File | Description |
|---|---|
docs/guides/adding-a-new-command.md |
Adds reliability guidance. |
cli/azd/AGENTS.md |
Documents testing and writer conventions. |
cli/azd/docs/recording-functional-tests-guide.md |
Expands functional-test isolation guidance. |
cli/azd/cmd/tool.go |
Routes spinner output to the action writer. |
cli/azd/pkg/extensions/manager.go |
Uses resilient extension cleanup. |
cli/azd/pkg/osutil/rename_unix.go |
Adds Unix RemoveAll. |
cli/azd/pkg/osutil/rename_windows.go |
Adds retrying Windows RemoveAll. |
cli/azd/pkg/osutil/rename_windows_test.go |
Tests locked-file cleanup. |
cli/azd/pkg/ux/confirm.go |
Propagates the confirm writer. |
cli/azd/pkg/ux/multi_select.go |
Propagates the multi-select writer. |
cli/azd/pkg/ux/prompt.go |
Propagates the prompt writer. |
cli/azd/pkg/ux/select.go |
Propagates the select writer. |
cli/azd/test/functional/cli_test.go |
Reuses shared resilient cleanup. |
cli/azd/test/functional/extension_test.go |
Isolates extension test state. |
cli/azd/test/functional/up_test.go |
Guards fixture type assertions. |
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 7
- Review effort level: Medium
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 73c24c8d-f132-424d-a406-e57aa0984576
d7d52b3 to
f924f6a
Compare
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
vhvb1989
left a comment
There was a problem hiding this comment.
Approving. Verified locally:
- Root-cause fix is correct and complete. All five
cli/azd/cmd/tool.gospinners now route through an injectedio.Writerinstead of defaulting toos.Stdout, anddetectAllToolstakes the writer from each action. This is the same fix I had drafted in #9184 (now closed in favor of this broader PR), and it also coversextension.goand thepkg/uxprompt/select/confirm paths. - The 7 phantom failures from #9181 pass on this branch (
cmd+cmd/middleware), and the spinner text no longer leaks into thego test -jsonstream. - Full CI is green, including the
azure-dev - cliWindows/Mac/Linux ADO pipeline that these phantom failures were reddening.
The remaining Copilot review comments are test-coverage suggestions (buffer-backed writer-propagation tests) rather than blocking defects — worth a follow-up but not gating. The Windows lock-retry cancellation note on extensions/manager.go is a reasonable tradeoff for the bounded retry window.
Also resolves #9181.
richardpark-msft
left a comment
There was a problem hiding this comment.
Look, I saw some stuff, I read some stuff, I liked it.
But some feedback, and just a recommendation to make sure our review skill is picking up the stuff you've added to our guides. (I think it does, but worth a double-check)
| - [ ] Start with `recording.Start(t)` | ||
| - [ ] Use `randomOrStoredEnvName(session)` | ||
| - [ ] Create CLI with `azdcli.WithSession(session)` | ||
| - [ ] Use a private `AZD_CONFIG_DIR` for user-level state changes; authenticate it if live Azure access is required |
There was a problem hiding this comment.
Given we're in (what I assume) is just a normal test, are you saying you actually need to set this before running the entire test suite (ie, in the shell), or set it before your first call to azd (ie, t.Setenv())?
There was a problem hiding this comment.
Neither shell-wide nor t.Setenv - set it on the test CLI's env before the first azd invocation. Clarified the wording and added a sample: cli.Env = append(cli.Env, "AZD_CONFIG_DIR="+configDir).
There was a problem hiding this comment.
Should this just always be done? For tests it seems like ensuring clean state is just good, overall.
There was a problem hiding this comment.
It's not the case right now so I'm hesitant to recommend that for all future functional tests
There was a problem hiding this comment.
If you were just deciding, though, what do you think? Is there a big overhead towards having a separate config dir per test?
| configDir := tempDirWithDiagnostics(t) | ||
| cli.Env = append(cli.Env, "AZD_CONFIG_DIR="+configDir) |
richardpark-msft
left a comment
There was a problem hiding this comment.
Added a "future thought" comment, but otherwise looks good. Thanks for addressing my feedback :)
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
jongio
left a comment
There was a problem hiding this comment.
Verified locally on Windows: go build and go vet clean for the touched packages (osutil, ux, extensions, cmd).
The core reliability changes hold up:
osutil.RemoveAllshares the retry backoff withRenameand threads context, so cancellation stops the loop instead of spinning for 10 seconds. The Unix side is a straight passthrough with the context kept for parity.Manager.Uninstall(ctx, ...)drops theMkdirAll-then-remove dance;os.RemoveAllon a missing dir is already a no-op, so behavior is unchanged and the extension dir cleanup now retries transient Windows locks.- Writer propagation reaches the nested cursor (
NewInput(w)->NewCursor(w), stdout fallback when unset), and the new writer tests actually exercise that path rather than the framework default. - The functional-test isolation via a private
AZD_CONFIG_DIRdoesn't drop any pre-set env, and the guarded fixture assertions fail the test instead of panicking the package.
No blocking issues from my pass.
Fixes #9162
This PR improves Windows test reliability by addressing the initiating extension cleanup race and the failures it caused in later tests.
Issue
Windows can briefly retain a lock on an extension executable after the process exits. The force-install test then failed to replace the extension, leaked global extension state, and caused unrelated tests to fail or prompt unexpectedly.
failed to remove extension: ... Access is deniedgo test -jsonstream.Changes
Testing
Covered the Windows lock retry, extension force-install flow, command JSON-stream integrity, UX writer behavior, and functional-test compilation with focused package and functional tests.